NGSIv2 rendering improvements (simplification, unification and performance) #3285

fgalan · 2018-09-05T12:05:35Z

This PR is motivated by #1298 (comment) and introduces improvements in NGSIv2 rendering logic in both simplfication/unification and peformance aspects. A summary of changes follow.

With regards to simplification/unification:

Extend JsonHelper utilization along NGSIv2 rendering logic
Simplification in several methods, mainly:
- CompoundValueNode::toJson()
- Entity::toJson() (spliting in several other methods)
- ContextAttribute::toJson()
- ContextAttributeVector:.toJson() (removed)
- MetadataVector::toJson() (the usage of JsonHelper removes the need of double-pass)
Simplification in argument passing. In particular, bool comma removed whenever possible
Removing stale code.

With regards to peformance:

JsonHelper implemention changed from std::ostringstream to std::string concatenation (as in previous NGSIv2 renderind code for entities). After the changes in this PR, I think std::ostringstream is no longer used in any part of the CB code.
Logic in the toJson() methods has been simplified.
I have done some basic ab testing with render intenstive operations (GET /v2/entities?limit=1000 and GET /v2/subscriptions?limit=1000, which 500 bytes entities/subscription, i.e. ~50KB in each GET response). The details of the test are here for master and here for this branch, but, in summary the result is pretty good:
- Entities rendering test: from 77.22 tps to 103.06 tps (33.46% gain)
- Subscriptions renderint test: 25.73 tps from to 119.75 tps (365.4% gain)

(The script used to populate the database can be found here. Eventually, it would be added to the repository for future usage)

From my point of view, this PR doesn't completes #1298 and more could to be done with regards to simplification and unification (for instance, ContextElement should use Entity internally and this will simplify a lot removing all the specific ContextElement rendering code) and peformance (other implementation alternatives for JsonHelper.cpp could be explored) but I think is an important step ahead.

Note, there are some changes in .test but all there seem to be ok. See specific comments in .test files.

…d simplicity

fgalan · 2018-09-05T12:07:29Z

src/lib/apiTypesV2/Attribute.cpp

@@ -29,6 +29,7 @@
 #include "common/errorMessages.h"


CNR entries missing

(Self-node)

Fixed in a99e4d6

fgalan · 2018-09-05T12:09:05Z

src/lib/common/string.h

-}
-
-template <> std::string toString(float f);
+extern std::string double2string(double f);


toString() was pretty generic... I think that double2string() is more clear.

Definitely.

NTC

fgalan · 2018-09-05T12:11:27Z

test/functionalTest/cases/0000_ipv6_support/ipv6_only.test

@@ -155,4 +155,4 @@ Date: REGEX(.*)
 brokerStop CB
 accumulatorStop ${LISTENER_PORT}
 accumulatorStop ${LISTENER2_PORT}
-#dbDrop CB


I did a general look for #dbDrop CB while I was testing and discover this old one. I have take the opportunity to fix it.

NTC (only informative)

fgalan · 2018-09-05T12:12:29Z

test/functionalTest/cases/0876_entity_dates/entity_dates_overridden_by_user_subs.test

@@ -462,6 +462,11 @@ Fiware-Correlator: REGEX([0-9a-f\-]{36})
                "type": "Text",
                "value": "bar"
            },
+            "dateModified": {


Old implemenation was buggy and dateModified were not included in the notification. After render changes, the bug has been solved, so this .test is changing.

NTC (I guess)

fgalan · 2018-09-05T12:12:43Z

test/functionalTest/cases/1179_options_test/GET_v2_ent_attr_val_options_text.test

@@ -58,7 +58,7 @@ echo
 echo


-echo "02. GET /v2/entities/E!/attrs/a_string/value"
+echo "02. GET /v2/entities/E1/attrs/a_string/value"


Small typo in echo line, now fixed.

NTC (I guess)

fgalan · 2018-09-05T12:15:59Z

test/functionalTest/cases/1367_time_measures/time_measure_stats.test

@@ -217,7 +217,7 @@ echo
 01. Second call to GET /statistics (gives time stat of FIRST request for /statistics)
 =====================================================================================
 HTTP/1.1 200 OK
-Content-Length: 127


I was having problems with Content-Length of statistics and metrics text. For instance, I see that 0.71234 changed to 0.6223 which stills matching the REGEX() for that value, but reduce the Content-Length in 1.

In general, Content-Length in statistics/metrics test is not very important. In fact, I have already seen many Content-Length: REGEX(.*) and Content-Length: REGEX(\d+). Thus, I understand the similar changes in this PR are ok.

yeah, no reason to include the exact Content-Length. the payload is there to be compared. more than enough.

NTC

fgalan · 2018-09-05T12:16:39Z

test/functionalTest/cases/2507_csub_metadata_field/previous_value_with_compounds.test

@@ -279,13 +279,13 @@ Fiware-Correlator: REGEX([0-9a-f\-]{36})
            "A": {
                "metadata": {
                    "previousValue": {
-                        "toplevel": [


This is funny! :)

It seems we have a bug in compound metadata rendeering logic. With the new implementaiton of the renders, now seems to be fixed and this .test has been modified to align with it.

Fix mentioned in CNR

NTC (I guess)

kzangeli

Looks very good.
Just a few minor comments

kzangeli · 2018-09-05T16:09:56Z

src/lib/apiTypesV2/Entity.cpp

+    {
+      // Reorder attributes in the same order they are in attrsFilter, excluding the ones
+      // note there (i.e. filtering them out) and giving special treatment to creation
+      // and modification dates


excluding the ones note there ...

Difficult to understand this phrase.
Is it supposed to be "excluding the ones *not * there"?
Doesn't make too much sense. Can't exclude something that is already not present ...

Perhaps:
"except those that aren't present, of course ..."

Rewritten in 295296b

kzangeli · 2018-09-05T16:12:42Z

src/lib/apiTypesV2/Entity.cpp

+        }
+      }
+
+      // All the remainder elements in attributeVector need to be released,


remainder -> remaining?

Fixed in 295296b

kzangeli · 2018-09-05T16:14:18Z

src/lib/apiTypesV2/Entity.cpp

+          if (found != -1)
+          {
+            caNewV.push_back(attributeVector.vec[found]);
+            attributeVector.vec.erase(attributeVector.vec.begin() + found);


Is this necessary?
Later, "attributeVector.release();" should take care of all of it, right?

If we don't erase() from the original vector, then the same caP will be pointed from two places (the old vector and the new vector) so it will freed twice, causing a double free.

I have a valgrind run in place to confirm if there is something wrong with regards to memory management in the current implementation.

ok, I guess ...
NYC

kzangeli · 2018-09-05T16:14:44Z

src/lib/apiTypesV2/Entity.cpp

+    }
+  }
+
+  // Legacy support for options=dateCreated and opations=dateModified


Typo: options?

Fixed in 295296b

kzangeli · 2018-09-05T16:15:17Z

src/lib/apiTypesV2/Entity.cpp

+    attributeVector.push_back(caP);
+  }
+
+  // Removing dateExpires if not explictely included in the filter


Typo: explictely

Fixed in 295296b

kzangeli · 2018-09-05T16:27:08Z

src/lib/ngsi/ContextAttributeVector.cpp

@@ -507,15 +339,15 @@ void ContextAttributeVector::fill(ContextAttributeVector* cavP, bool useDefaultT
 *
 * lookup -
 */
-ContextAttribute* ContextAttributeVector::lookup(const std::string& attributeName) const
+int ContextAttributeVector::lookup(const std::string& attributeName) const


A better name of this method would be 'index' or 'getIndex'.
'lookup' indicates to actually return what was looked up ...

Good point. Chaning name.

Fixed in 295296b

kzangeli · 2018-09-05T16:28:05Z

src/lib/ngsi/ContextElement.cpp

+    }
+  }
+
+  // Removing dateExpires if not explictely included in the filter


typo: explicitly

Fixed in 295296b

kzangeli · 2018-09-05T16:32:05Z

src/lib/ngsi/MetadataVector.cpp

-  }
-
-  return out;
+  return jh.str();
 }


Truly great would be to have an input parameter "JsonHelper* jhP" to all these rendering functions, instead of returning objects on the stack ...

NTC, but ... perhaps in a future modification? ;-)

You mean to have a single instance of JsonHelper per rendered tree passing it down along the tree?

An interesting idea, although it should be explored in terms of complexity / performance gain. Perhaps in the future...

Yes, one single JsonHelper per response. that would be good.
As you say, perhaps in the future ...
Issue?

NTC

kzangeli · 2018-09-05T16:33:07Z

src/lib/parse/CompoundValueNode.cpp


-  if (container != NULL)
+  switch(valueType)


Space missing. 'switch' is not a function ...

Fixed in 295296b

kzangeli · 2018-09-05T16:35:30Z

test/functionalTest/cases/1367_time_measures/time_measure_stats.test

@@ -217,7 +217,7 @@ echo
 01. Second call to GET /statistics (gives time stat of FIRST request for /statistics)
 =====================================================================================
 HTTP/1.1 200 OK
-Content-Length: 127


yeah, no reason to include the exact Content-Length. the payload is there to be compared. more than enough.

NTC

kzangeli · 2018-09-06T10:43:36Z

LGTM (assuming valgrind reports no leaks)

…efonicaid/fiware-orion into hardening/1298_render_simplification

…essing logic

fgalan · 2018-09-07T06:42:49Z

Some additional changes were required after LGTM, due to memory management issues. The new changes can be seen at 6eafb3f...7a31172

fgalan · 2018-09-07T06:45:10Z

test/functionalTest/cases/0000_ngsi10_convenience/subscriptions_onchange.test

@@ -38,8 +38,8 @@ payload='{
        }
    ],
    "attributes": [
-        "temperature",
-        "ligthstatus"
+        "ligthstatus",


With the changes in 6eafb3f...7a31172 the "attributes" vector now defines the order of the attributes as they have to appear in the response. I see this as a little improvement, but the .test needs this slight adjust.

kzangeli

LGTM

fgalan added 2 commits September 4, 2018 21:50

FIX refactor of NGSIv2 rendering method for the sake of homogenity an…

65533d9

…d simplicity

FIX comments in the code

d34e43f

fgalan commented Sep 5, 2018

View reviewed changes

FIX code style

1a5045e

fgalan mentioned this pull request Sep 5, 2018

JsonHelper usage in all NGSIv2 renders (second step: optimization and performance) #1298

Open

ADD CNR entries

a99e4d6

kzangeli suggested changes Sep 5, 2018

View reviewed changes

fgalan and others added 2 commits September 5, 2018 21:13

FIX issues raised in code review

295296b

Merge branch 'master' into hardening/1298_render_simplification

6eafb3f

fgalan mentioned this pull request Sep 6, 2018

ADD populate_500bytes_entities_and_subs.py script #3286

Merged

fgalan added 3 commits September 6, 2018 16:20

FIX memory leak

c9faf32

Merge branch 'hardening/1298_render_simplification' of github.com:tel…

1939f14

…efonicaid/fiware-orion into hardening/1298_render_simplification

FIX filterAttribute() must be invoked before in the notification proc…

7a31172

…essing logic

fgalan commented Sep 7, 2018

View reviewed changes

fgalan added 3 commits September 7, 2018 11:48

FIX broken unit tests due to function signature change

f4bfb3d

FIX code style

d213ea4

FIX broken utests

3ba239f

This was referenced Sep 10, 2018

[Number precision] JSON timestamp wrong on FIWARE Orion Context Broker global instance #2603

Closed

[Number precision] Improve precision for decimalDigits() function #2425

Closed

kzangeli approved these changes Sep 12, 2018

View reviewed changes

AlvaroVega approved these changes Sep 12, 2018

View reviewed changes

AlvaroVega merged commit 8f2e123 into master Sep 12, 2018

fgalan deleted the hardening/1298_render_simplification branch September 12, 2018 15:53

This was referenced Sep 12, 2018

.test for bugfix 3280 illegal json fix #3291

Merged

Orion returns illegal JSON in some situations #3280

Closed

This was referenced Sep 21, 2018

Unify ContextElement class into Entity class #3301

Merged

Bad encoding of end of line characters #2938

Closed

NGSIv2 rendering improvements (simplification, unification and performance) #3285

NGSIv2 rendering improvements (simplification, unification and performance) #3285

Conversation

fgalan commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzangeli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fgalan Sep 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzangeli commented Sep 6, 2018

fgalan commented Sep 7, 2018

Choose a reason for hiding this comment

kzangeli left a comment

Choose a reason for hiding this comment

fgalan Sep 5, 2018 •

edited

Loading